🐛 Fix CtrlOp::getBodyUnitary() for operations with parameters #1464

taminob · 2026-01-15T14:37:42Z

Description

While debugging test cases in #1426, I finally figured out why CtrlOp::getBodyUnitary() sometimes returned an invalid UnitaryOpInterface.
Whenever an operation has at least one parameter, the first operation in the modifier's body will be a arith.constant and not the unitary operation like it is currently assumed in the function.

This PR resolves this by iterating over all operations in the body and only returning when the first UnitaryOpInterface operation has been found or there is none.
It affects all crx, crzz, cu, ... operations.

Required for #1426

Checklist:

The pull request only contains commits that are focused and relevant to this change.
I have added appropriate tests that cover the new/changed functionality.
I have updated the documentation to reflect these changes.
I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
I have added migration instructions to the upgrade guide (if needed).
The changes follow the project's style guidelines and introduce no new warnings.
The changes are fully tested and pass the CI checks.
I have reviewed my own code changes.

coderabbitai · 2026-01-15T14:39:33Z

📝 Walkthrough

Summary by CodeRabbit

Documentation
- Updated CHANGELOG with additional MLIR dialect infrastructure PR references.
Tests
- Added test coverage for QCO control operations with parameterized unitaries.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

This PR adds two PR references to CHANGELOG.md within the MLIR dialect infrastructure section and introduces a new unit test case bodyUnitaryWithParameter to verify CtrlOp's body unitary functionality with parameterized gates.

Changes

Cohort / File(s)	Summary
Documentation Updates `CHANGELOG.md`	Added PR references [`#1464`] and [`#1465`] to the unreleased MLIR dialect infrastructure items list (+2/-1)
Unit Tests `mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp`	Added new test case `bodyUnitaryWithParameter` that verifies CtrlOp correctly extracts and validates body unitary from a controlled CRX gate on a 2-qubit register (+13)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

#1436: The new test targets CtrlOp behavior and implementation areas modified by this PR
#1430: This PR changes CtrlOp region-accessing methods that the new test interacts with (e.g., getBody(), getBodyUnitary)

Suggested reviewers

burgholzer

Poem

🐰 A test hops in, with qubit care,
Checking unitaries floating there,
And CHANGELOG sings the PR song—
Documentation marching strong! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description provides a clear explanation of the bug, the fix, and why it was needed. However, the checklist indicates that changelog entries and documentation updates were not completed as required by the template.	Complete the unchecked items in the checklist: add changelog entries for this fix and update documentation if applicable. Clarify CI testing status if not yet fully passed.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main bug fix: correcting CtrlOp::getBodyUnitary() to handle operations with parameters, which is the core change across all modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d91cf85 and 5fae654.

📒 Files selected for processing (2)

CHANGELOG.md
mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

🧰 Additional context used

🧠 Learnings (16)

📓 Common learnings

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.

Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.

Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.

📚 Learning: 2026-01-08T22:56:09.502Z

Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1435
File: docs/mlir/QC.md:1-11
Timestamp: 2026-01-08T22:56:09.502Z
Learning: In the munich-quantum-toolkit/core repository, MLIR dialect documentation files (e.g., MLIRQCDialect.md, MLIRQCInterfaces.md) are automatically generated during the documentation build via the ReadTheDocs `pre_build` step and do not need to be committed to the repository.

Applied to files:

CHANGELOG.md

📚 Learning: 2025-12-05T17:45:37.602Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.

Applied to files:

CHANGELOG.md

📚 Learning: 2026-01-10T18:49:44.352Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.

Applied to files:

CHANGELOG.md
mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-10-14T14:37:38.047Z

Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.

Applied to files:

CHANGELOG.md

📚 Learning: 2025-12-08T23:58:09.648Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-12-08T12:44:05.883Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-10-09T13:13:51.224Z

Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-12-08T14:55:43.899Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2026-01-12T12:05:56.683Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp:599-757
Timestamp: 2026-01-12T12:05:56.683Z
Learning: In the QCO builder (mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp), the SCF and func builder methods (scfFor, scfWhile, scfIf, funcCall) assume that all operands passed to them are qubit types (qco.qubit). The current validation through updateQubitTracking is sufficient for this use case. The same assumption applies to the QC builder methods.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2026-01-07T12:29:02.062Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1040-1050
Timestamp: 2026-01-07T12:29:02.062Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), ConvertQCOFuncCallOp assumes that if a func::CallOp has qubit results, then all arguments and results are qubits (no mixed classical/quantum types). The conversion is scoped to handle all-qubit function calls only.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2026-01-10T16:28:41.975Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1729-1763
Timestamp: 2026-01-10T16:28:41.975Z
Learning: In the QCToQCO conversion pass (mlir/lib/Conversion/QCToQCO/QCToQCO.cpp), the dynamic legality checks for func operations (func::CallOp, func::FuncOp, func::ReturnOp) and scf operations assume that operations use either all classical types or all qubit types, never mixed. Therefore, checking for the presence of qc::QubitType in operands or arguments is sufficient to determine if conversion is needed—there is no need to check both operands and results separately.
<!-- </add_learning]

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2026-01-07T12:29:16.380Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1070-1085
Timestamp: 2026-01-07T12:29:16.380Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), the ConvertQCOFuncFuncOp pattern assumes that when a func.func operation is matched for conversion, all of its arguments are qco.qubit types (never mixed qubit/classical). The pattern unconditionally converts all arguments to qc::QubitType based on this assumption.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2026-01-10T16:07:55.896Z

Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:988-1024
Timestamp: 2026-01-10T16:07:55.896Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), the SCF operation conversion patterns (ConvertQCOScfYieldOp, ConvertQCOScfConditionOp, ConvertQCOScfIfOp, ConvertQCOScfWhileOp, ConvertQCOScfForOp) assume that all operands are qubit types (qco.qubit or qc.qubit), never mixed qubit/classical types. The conversion is scoped to handle all-qubit SCF operations only.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-12-08T23:44:39.930Z

Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

📚 Learning: 2025-12-17T11:32:45.843Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.

Applied to files:

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

GitHub Check: 🇨‌ Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
GitHub Check: 🇨‌ Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
GitHub Check: 🇨‌ Test 🍎 (macos-15, clang, Release) / 🍎 macos-15 clang Release
GitHub Check: 🇨‌ Test 🍎 (macos-15, clang, Debug) / 🍎 macos-15 clang Debug
GitHub Check: 🇨‌ Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
GitHub Check: 🇨‌ Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
GitHub Check: 🇨‌ Test 🏁 (windows-2025, msvc, Release) / 🏁 windows-2025 msvc Release
GitHub Check: 🇨‌ Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
GitHub Check: 🇨‌ Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
GitHub Check: 🇨‌ Coverage / 📈 Coverage
GitHub Check: 🇨‌ Lint / 🚨 Lint
GitHub Check: Summary

🔇 Additional comments (3)

mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp (1)
225-236: LGTM! Well-designed regression test for the getBodyUnitary() fix.

This test effectively validates the bug fix by using crx which generates an arith.constant op before the RxOp in the control body, exactly reproducing the scenario where the old implementation would fail.

Consider adding explicit IR verification for consistency with other tests in this file (e.g., LambdaBuilder, UnitaryOpBuilder):
💡 Optional: Add explicit verification
   auto bodyUnitary = ctrlOp.getBodyUnitary();
   // Test if a valid unitary operation is returned
   ASSERT_TRUE(bodyUnitary);
   // Ensure it contains the correct operation type
   EXPECT_EQ(bodyUnitary.getBaseSymbol(), "rx");
+
+  // Verify operation
+  EXPECT_TRUE(mlir::verify(ctrlOp).succeeded());
CHANGELOG.md (2)

14-14: LGTM!

The PR reference is correctly added to the MLIR dialect infrastructure entry, following the established format and ordering.

318-318: LGTM!

PR link definition correctly added in descending order.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-01-15T14:43:38Z

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp	66.6%	1 Missing ⚠️
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp	75.0%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

burgholzer

I am not 100% that this is the right solution here.
The IR for parasitized operations shouldn't actually pass verification because the verifier of the QC) CtrlOp asserts that any CtrlOp only contains two operations (the UnitaryOp and the yield). I am surprised that we are not hitting this so far.
I assume that we should be running mlir::verify in the compiler pipeline tests on all circuits constructed by any builders or produced by the compiler.

I would argue that the real fix here is to modify the builders so that they directly insert the constant operations in the highest scope possible.
This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

My request here would be two-fold

Can we fix the builders instead of iterating over the control op body?
Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

taminob · 2026-01-16T18:35:13Z

This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

What's the argument against just moving the parameter into the body? Wouldn't that also simplify optimizations involving the control modifier since it actually is IsolatedFromAbove?

* Can we fix the builders instead of iterating over the control op body?

Do you have an idea how we could define this "highest scope possible"? Because wouldn't that be the ModuleOp, but we probably want it in the FuncOp instead?

* Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

I guess that should be possible, but how do we want to handle that in actual non-test code? Running a verification after every optimization would probably cause a lot of unnecessary overhead. Or do we just assume that the optimizations are all doing valid transformations? Because it could also happen that "optimization 1" will cause an invalid IR which "optimization 2" will transform back into a valid IR but with a different functionality than intended and thus only the final IR check might be insufficient?

burgholzer · 2026-01-16T19:03:17Z

This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

What's the argument against just moving the parameter into the body? Wouldn't that also simplify optimizations involving the control modifier since it actually is IsolatedFromAbove?

There might be multiple uses of the same parameter across operations and we do not want to bury these in (potentially nested) modifier regions.
Parameters might originate from the results of complex computations, so you will never be able to move them into the control regions in every use case.

* Can we fix the builders instead of iterating over the control op body?

Do you have an idea how we could define this "highest scope possible"? Because wouldn't that be the ModuleOp, but we probably want it in the FuncOp instead?

The canonicalization pipeline that we employ as part of the compiler pipeline already employs this "bubbling up as high as possible". You can see that in the various stages of the IR in the tests. This should also give you an indication of where the appropriate place for the constant ops is. I believe it should be the first regions with with IsolatedFromAbove trait (which might be the ModuleOp).
Maybe the trick here is to ensure that canonicalization can pull up the constants originally defined within modifier operations.

* Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

I guess that should be possible, but how do we want to handle that in actual non-test code? Running a verification after every optimization would probably cause a lot of unnecessary overhead. Or do we just assume that the optimizations are all doing valid transformations? Because it could also happen that "optimization 1" will cause an invalid IR which "optimization 2" will transform back into a valid IR but with a different functionality than intended and thus only the final IR check might be insufficient?

See https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-should-be-valid-before-and-after-each-pass and https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier
This is actually enforced by MLIR by default when running passes with a pass manager.
Which makes me slightly confused whether there actually is something to fix here, or whether the implementation is working as intended.

taminob · 2026-01-16T19:20:10Z

See https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-should-be-valid-before-and-after-each-pass and https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier
This is actually enforced by MLIR by default when running passes with a pass manager.
Which makes me slightly confused whether there actually is something to fix here, or whether the implementation is working as intended.

Thanks for the links! I need to do some reading and experimenting, but there definitely is something broken here because getUnitaryBody() will try to return a arith::ConstantOp (but since it is dyn_cast<UnitaryOpInterface>, it will return nullptr instead).
Not really relevant to the unitary matrix PR if I simply remove these gates from the test cases, but needs to be fixed anyway, I guess, since that is a rather severe issue in my opinion (and can cause some headache as I can attest 😅).

Edit: I also made sure that the test case I added will fail without my proposed fix, that is why I'm sure it currently is broken

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

burgholzer · 2026-01-16T19:30:11Z

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

We are currently not seeing them in the compiler pipeline tests, but this could very much be due to us not having a test with controlled rotations (which we should add on that case).
Based on the discussion above I would assume that we don't have such a test yet and if we add one to the compiler pipeline tests we would observe the error.

This reverts the following commits: * 5a3f2de * 47b6798

taminob · 2026-01-16T19:39:31Z

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added ASSERT_TRUE(mlir::verify(*module).succeeded()); to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

burgholzer · 2026-01-16T19:43:19Z

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Thanks.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added ASSERT_TRUE(mlir::verify(*module).succeeded()); to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

Yeah. Seams like a bug in the builder to me. Or in our definition of a valid control modifier, but I am heavily leaning to the bug.

@denialhaag could you maybe briefly take a look at this?

denialhaag · 2026-01-16T20:51:32Z

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Thanks.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added `ASSERT_TRUE(mlir::verify(*module).succeeded());` to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

Yeah. Seams like a bug in the builder to me. Or in our definition of a valid control modifier, but I am heavily leaning to the bug.

@denialhaag could you maybe briefly take a look at this?

I am all but certain that the issue is that we are not resolving the std::variant before the call to CtrlOp::create here:

core/mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp

Lines 227 to 234 in 0089f7d

    
           QCProgramBuilder& QCProgramBuilder::mc##OP_NAME(                             \ 
        
               const std::variant<double, Value>&(PARAM), ValueRange controls,          \ 
        
               Value target) {                                                          \ 
        
             checkFinalized();                                                          \ 
        
             CtrlOp::create(*this, controls,                                            \ 
        
                            [&] { OP_CLASS::create(*this, target, PARAM); });           \ 
        
             return *this;                                                              \ 
        
           }

Instead, the std::variant is resolved here (i.e., inside the control modifier) via the call to variantToValue:

core/mlir/lib/Dialect/QC/IR/Operations/StandardGates/RXOp.cpp

Lines 22 to 26 in 0089f7d

    
           void RXOp::build(OpBuilder& builder, OperationState& state, Value qubitIn, 
        
                            const std::variant<double, Value>& theta) { 
        
             auto thetaOperand = variantToValue(builder, state.location, theta); 
        
             build(builder, state, qubitIn, thetaOperand); 
        
           }

I think we could simply call the variantToValue function also before CtrlOp::create. I will push the necessary fixes to this PR tomorrow!

burgholzer · 2026-01-16T20:56:34Z

I am all but certain that the issue is that we are not resolving the std::variant before the call to CtrlOp::create here:

core/mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp

Lines 227 to 234 in 0089f7d

QCProgramBuilder& QCProgramBuilder::mc##OP_NAME( \

const std::variant<double, Value>&(PARAM), ValueRange controls, \

Value target) { \

checkFinalized(); \

CtrlOp::create(*this, controls, \

[&] { OP_CLASS::create(*this, target, PARAM); }); \

return *this; \

}

Instead, the std::variant is resolved here (i.e., inside the control modifier) via the call to variantToValue:

core/mlir/lib/Dialect/QC/IR/Operations/StandardGates/RXOp.cpp

Lines 22 to 26 in 0089f7d

void RXOp::build(OpBuilder& builder, OperationState& state, Value qubitIn,

const std::variant<double, Value>& theta) {

auto thetaOperand = variantToValue(builder, state.location, theta);

build(builder, state, qubitIn, thetaOperand);

}

I think we could simply call the variantToValue function also before CtrlOp::create. I will push the necessary fixes to this PR tomorrow!

Indeed, that seems to be the main issue and a nice solution to the issue! Thanks for quickly getting back to this.

taminob self-assigned this Jan 15, 2026

taminob added bug Something isn't working fix Fix for something that isn't working c++ Anything related to C++ code MLIR Anything related to MLIR labels Jan 15, 2026

coderabbitai bot approved these changes Jan 15, 2026

View reviewed changes

taminob requested a review from burgholzer January 15, 2026 14:45

taminob changed the title ~~Fix CtrlOp::getBodyUnitary() for operations with parameters~~ 🐛 Fix CtrlOp::getBodyUnitary() for operations with parameters Jan 15, 2026

mergify bot added the conflict label Jan 16, 2026

burgholzer requested changes Jan 16, 2026

View reviewed changes

taminob added 5 commits January 16, 2026 20:34

🐛 Fix qco::CtrlOp::getBodyUnitary() for ops with params

aaa00a1

🐛 Fix qc::CtrlOp::getBodyUnitary() for ops with params

175ad4d

✅ Add test case for qco::CtrlOp::getBodyUnitary()

d8c6488

📝 Add PR to CHANGELOG

6d88bb6

Revert ":bug: Fix qc(o)::CtrlOp::getBodyUnitary() for ops with params"

5fae654

This reverts the following commits: * 5a3f2de * 47b6798

taminob force-pushed the taminob/mlir-qco-fix-ctrl-get-body-unitary branch from cc9488f to 5fae654 Compare January 16, 2026 19:34

mergify bot removed the conflict label Jan 16, 2026

burgholzer removed the bug Something isn't working label Jan 16, 2026

Uh oh!

🐛 Fix CtrlOp::getBodyUnitary() for operations with parameters #1464

Are you sure you want to change the base?

🐛 Fix CtrlOp::getBodyUnitary() for operations with parameters #1464

Conversation

taminob commented Jan 15, 2026

Description

Checklist:

Uh oh!

coderabbitai bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

codecov bot commented Jan 15, 2026

Codecov Report

Uh oh!

burgholzer left a comment

Choose a reason for hiding this comment

Uh oh!

taminob commented Jan 16, 2026

Uh oh!

burgholzer commented Jan 16, 2026

Uh oh!

taminob commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burgholzer commented Jan 16, 2026

Uh oh!

taminob commented Jan 16, 2026

Uh oh!

burgholzer commented Jan 16, 2026

Uh oh!

denialhaag commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burgholzer commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai bot commented Jan 15, 2026 •

edited

Loading

taminob commented Jan 16, 2026 •

edited

Loading

denialhaag commented Jan 16, 2026 •

edited

Loading